feat: surface stale-CODEOWNERS diff as info output instead of inline error#108
feat: surface stale-CODEOWNERS diff as info output instead of inline error#108dkisselev wants to merge 1 commit into
Conversation
…error The diff added in #107 was rendered inline under the "CODEOWNERS out of date" headline, so a long diff pushed the actionable line up and out of view in CI logs, and the wrapping code_ownership gem raised the entire diff as part of the error. Route the diff through RunResult.info_messages instead. main.rs prints info_messages ahead of validation errors and still exits 1, so the diff prints first as informational output while the terse, actionable headline lands at the bottom of the log where it's seen. Errors::messages() no longer emits the diff; a new Errors::info_messages() extracts it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dduugg
left a comment
dduugg
left a comment
There was a problem hiding this comment.
Approve with nits. Clean, well-targeted follow-up to #107. It relocates the stale diff out of Error::messages() into a new Errors::info_messages(), wires it through Runner::validate_all into RunResult.info_messages, and leans on main.rs::maybe_print_errors already printing info messages before validation errors (exit code stays 1). Idiomatic, well-documented, tests updated correctly for the new ordering.
Key design question: stdout vs stderr
The PR's own Note is admirably honest: this does not achieve the second stated goal (the wrapping code_ownership gem raising the whole diff). Both info_messages and validation_errors go to stdout (src/main.rs:19 and :27), so a gem capturing all stdout still captures the full diff, just reordered. This PR fixes log ordering/readability, not captured-payload size.
Recommendation: ship it for the readability win, but be explicit in the merge note that the gem-payload problem is unresolved. If shrinking the raised payload is the real objective, the diff must go to stderr, and info_messages is the wrong vehicle, because maybe_print_errors routes info messages to stdout for every command (for-team, for-file, crosscheck). Overloading that field for a validate-only stderr path would be inconsistent; it deserves its own channel. Treat stderr as a deliberate follow-up, not something to bolt on here.
Findings
Low — validate_all info_messages wiring has no direct test (src/runner.rs:130)
The new info_messages: err.info_messages() line is covered only transitively via the two stdout golden tests. tests/runner_api.rs already asserts directly on RunResult fields against fixtures, so a struct-level assertion (stale project => non-empty info_messages containing the diff and non-empty validation_errors with the headline) is cheap and pins the wiring independent of stdout formatting.
Nits (optional):
- No direct unit test for
info_messages()(src/ownership/validator.rs:226). Well-covered by integration tests (esp.invalid_project_test.rs, where the stale error coexists with three other categories), so this is hygiene, not a gap. - Empty-diff guard is effectively unreachable (
validator.rs:230,if !diff.is_empty()).CodeownershipFileIsStaleis only constructed whengenerated_file != current_file, so the diff is always non-empty. Harmless (it mirrors the pre-PR guard), just dead defensive code. maybe_print_errorsordering not unit-tested (src/main.rs:16). The diff-before-headline invariant is pinned only as a golden-test side effect. A focused test would need the ordering extracted into a pure helper (the function callsprocess::exit), nice-to-have, not worth it for this PR alone.
What it does well
- Correct architectural home:
info_messages()on theErrorscollection parallels the existingDisplayimpl and matches howrunner.rsconsumes the collection. filter_map+ match-guard faithfully preserves the old!diff.is_empty()behavior; the retaineddifffield isn't dead, it's re-sourced as the info message's input.- Genuinely excellent doc comments explaining the why.
- Integration tests updated consistently with the project's golden-output convention.
- Transparent PR description about the unsolved gem-capture limitation.
2 participants
Summary
Follow-up to #107. That PR appended the stale-file diff to the
CODEOWNERS out of datevalidation error itself. Two consequences:Run \…` to update the CODEOWNERS file` (and any other validation categories) far up the CI log.code_ownershipRuby gemraises the returned message, so the entire diff lands inside the stack trace.This PR keeps the diff but moves it out of the error and into
RunResult.info_messages.How it works
main.rs::maybe_print_errorsalready printsinfo_messagesbefore the validation errors and stillprocess::exit(1). So:1).Before (#107)
After
Implementation
Error::messages()no longer emits the diff forCodeownershipFileIsStale(back tovec![]).Errors::info_messages()extracts the diff(s) as informational strings.Runner::validate_allpopulatesRunResult.info_messagesfrom it alongside the (now terse)validation_errors.difffield on the error variant is retained, just sourced differently.Tests
executable_name_config_test.rs,invalid_project_test.rs) for the new ordering — diff block first, headline after.codeowners_diffunit tests are unchanged.All tests,
clippy --all-targets --all-features -D warnings, andfmt --checkpass (enforced by the pre-commit hook).Note
This does not fully shrink what the gem raises: both
info_messagesandvalidation_errorsgo to stdout, so if the gem captures all of stdout the diff is still in the captured text. It does fix log ordering/readability. Genuinely shrinking the raised error would require routing the diff to stderr (a separate channel) — happy to take that direction instead if preferred.🤖 Generated with Claude Code